node/core/executor.go (1)

347-353: Inconsistent terminology: error message still references "MPT".

The error message at line 351 references "MPT-compatible geth" while the rest of this PR refactors to "Jade fork" terminology. Similar issue at lines 390-391.

Consider updating for consistency, or clarify if "MPT" is intentionally retained for user-facing messages.

♻️ Suggested terminology update

 		e.logger.Error("!!! CRITICAL: Geth is behind - node BLOCKED !!!",
 			"consensus_block", wrappedBlock.Number,
 			"geth_height", height,
-			"action", "Switch to MPT-compatible geth IMMEDIATELY")
+			"action", "Switch to Jade-compatible geth IMMEDIATELY")

And at lines 390-391:

-		e.logger.Error("HINT: If this occurs after MPT upgrade, your geth node may not support MPT blocks. " +
-			"Please ensure you are running an MPT-compatible geth node.")
+		e.logger.Error("HINT: If this occurs after Jade upgrade, your geth node may not support Jade blocks. " +
+			"Please ensure you are running a Jade-compatible geth node.")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@node/core/executor.go` around lines 347 - 353, Update the user-facing error
strings to use the new "Jade fork" terminology instead of "MPT" for consistency:
modify the logger.Error calls that reference "MPT-compatible geth" (the call
that logs "!!! CRITICAL: Geth is behind - node BLOCKED !!!" using
wrappedBlock.Number, height and returns types.ErrWrongBlockNumber) and the
similar messages around the block checks at the later error paths (the
logger.Error usage around lines ~390-391) so they mention "Jade fork" (or the
agreed new phrase) consistently; keep the same context parameters
(consensus_block, geth_height, action) and return types.ErrWrongBlockNumber as
before.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

node/sync/bridge_client.go (1)

27-30: Optional: Consider returning a read-only interface instead of concrete *ethclient.Client.

While the current callers only read from the returned client and no .Close() calls are present in the codebase, returning a concrete type exposes unnecessary mutability. The existing comment documents the intent ("for sharing with other services"), but narrowing the API surface to a read-only interface would make ownership boundaries clearer and prevent accidental misuse by future callers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@node/sync/bridge_client.go` around lines 27 - 30, Change
BridgeClient.L1Client to return a narrow read-only interface instead of
*ethclient.Client: declare a new interface (e.g., ReadOnlyEthClient or
L1ClientReader) in the same package that lists only the methods callers actually
use, update the method signature func (c *BridgeClient) L1Client()
ReadOnlyEthClient, and return c.l1Client (ethclient.Client implements the
interface). This preserves behavior but prevents callers from calling mutating
methods like Close(); ensure the interface includes all methods currently
invoked on the returned client so the code continues to compile.

ops/docker-sequencer-test/run-test.sh (4)

471-471: Fix inconsistent indentation.

Line 471 has an extra leading space before echo.

♻️ Proposed fix

-     echo "Node 0 (seq-0): Block $(get_block_number http://127.0.0.1:8545)"
+    echo "Node 0 (seq-0): Block $(get_block_number http://127.0.0.1:8545)"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/run-test.sh` at line 471, The echo line has an
extra leading space causing inconsistent indentation; remove the extra leading
space so the line reads starting at column 1 (i.e., align `echo "Node 0 (seq-0):
Block $(get_block_number http://127.0.0.1:8545)"` with surrounding lines),
keeping the same content and using the existing `get_block_number` call.

---

274-274: Document the well-known test private key.

This is the Anvil/Hardhat account #0 private key (a widely-used development key). Add a comment to clarify this is intentional and test-only.

📝 Proposed documentation

+    # Anvil/Hardhat account `#0` - well-known test key, DO NOT use in production
     export SEQUENCER_PRIVATE_KEY="0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/run-test.sh` at line 274, The exported
SEQUENCER_PRIVATE_KEY value in run-test.sh is the well-known Anvil/Hardhat
account `#0` private key used for local/dev testing; add an inline comment next to
the export of SEQUENCER_PRIVATE_KEY clarifying that this is the intentional,
widely-used development/test-only key (not for production) so readers understand
it's safe and expected in tests.

---

68-68: Shellcheck SC2155: Declare and assign separately.

Multiple lines combine local declaration with command substitution, which masks return values. While this is low-risk for a test script, separating declaration and assignment improves robustness.

♻️ Example fix pattern

-    local current=$(get_block_number "$rpc_url")
+    local current
+    current=$(get_block_number "$rpc_url")

Also applies to: 305-305, 313-314, 333-333, 342-342, 345-346, 360-360, 378-378

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/run-test.sh` at line 68, Several local variables
are declared with command substitution in one step (e.g., local
current=$(get_block_number "$rpc_url")), which triggers ShellCheck SC2155;
change each to declare the variable first and assign separately to avoid masking
return values. For each occurrence (functions like get_block_number and
variables such as current, target, next, etc.), replace the single-line "local
var=$(...)" with two lines: "local var" followed by "var=$(...)" ensuring you
preserve any quoting and command arguments (e.g., "$rpc_url"); apply this
pattern to all listed occurrences.

---

142-240: Embedded Python script is complex but functional.

The inline Python for contract deployment and genesis generation is extensive. Consider extracting this to a separate Python script file for better maintainability and testability. However, keeping it inline does ensure the script is self-contained.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/run-test.sh` around lines 142 - 240, Extract the
large inline Python payload into a separate script (e.g., generate_genesis.py)
and replace the embedded python3 -c "..." block in run-test.sh with a call to
that script; move all top-level symbols and logic (sys.path.insert(0,
'$DEVNET_DIR'), imports devnet, functions/variables like run_command, read_json,
write_json, log, devnet_cfg_orig, temp_deploy_config, genesis_batch_header,
addresses, etc.) into the new file, accept required environment values
(DEVNET_DIR, MORPH_ROOT, OPS_DIR, CONTRACTS_DIR or pass them as args/env), add a
proper shebang and make it executable, and ensure the shell script invokes
python3 generate_genesis.py with the same environment so return codes and
side-effects (files written to devnet_dir, deployment_dir, .env updates) are
preserved.

ops/docker-sequencer-test/docker-compose.override.yml (2)

3-3: version field is obsolete in Docker Compose v2+.

The version key is ignored by modern Docker Compose and produces a warning. Consider removing it.

♻️ Proposed fix

 # Override file to use test images
 # Copy this to ops/docker/docker-compose.override.yml before running test
-version: '3.8'
 
 services:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/docker-compose.override.yml` at line 3, Remove the
obsolete top-level "version:" key from docker-compose.override.yml (the
"version: '3.8'" line) because Compose v2+ ignores it and emits a warning;
simply delete that key and its value so the file uses the newer schema without
the version field.

---

27-27: Document that these are test-only private keys.

Gitleaks flagged these as potential API key leaks. While these appear to be pre-generated test keys (similar to well-known Anvil/Hardhat development keys), adding a comment will prevent future confusion and silence security scanners.

📝 Proposed documentation

   node-0:
     image: morph-node-test:latest
     build:
       context: ../..
       dockerfile: ops/docker-sequencer-test/Dockerfile.l2-node-test
     environment:
+      # Test-only sequencer key - DO NOT use in production
       - MORPH_NODE_SEQUENCER_PRIVATE_KEY=0xd99870855d97327d20c666abc78588f1449b1fac76ed0c86c1afb9ce2db85f32

   node-1:
     image: morph-node-test:latest
     environment:
+      # Test-only sequencer key - DO NOT use in production
       - MORPH_NODE_SEQUENCER_PRIVATE_KEY=0x0890c388c3bf5e04fee1d8f3c117e5f44f435ced7baf7bfd66c10e1f3a3f4b10

Also applies to: 36-36

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/docker-compose.override.yml` at line 27, Add an
inline comment next to the MORPH_NODE_SEQUENCER_PRIVATE_KEY environment variable
to clearly state that this value is a pre-generated test/development private key
(not for production) so scanners and future readers know it's safe to commit;
update the same comment for the other identical key occurrence (the second
MORPH_NODE_SEQUENCER_PRIVATE_KEY entry) to match.

ops/docker-sequencer-test/Dockerfile.l2-geth-test (1)

12-17: Consider using a minimal runtime image.

The runtime stage uses the full go-ubuntu-builder image which includes Go toolchain and build dependencies. For a smaller image, consider using a minimal base like ubuntu:22.04 or debian:bookworm-slim since only the compiled binary is needed at runtime.

💡 Example using minimal base

 # Runtime stage
-FROM ghcr.io/morph-l2/go-ubuntu-builder:go-1.24-ubuntu
+FROM ubuntu:22.04

 RUN apt-get -qq update && apt-get -qq install -y --no-install-recommends \
     ca-certificates bash curl \
     && rm -rf /var/lib/apt/lists/*

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/Dockerfile.l2-geth-test` around lines 12 - 17, The
runtime stage currently uses the full builder image
(ghcr.io/morph-l2/go-ubuntu-builder:go-1.24-ubuntu); replace it with a minimal
runtime base such as ubuntu:22.04 or debian:bookworm-slim, install only the
runtime packages (ca-certificates, bash, curl) as already present, and adjust
any subsequent COPY/USER instructions that expect the built binary (ensure the
compiled binary built in the builder stage is copied into the new minimal stage
and ownership/permissions are set appropriately). Keep the existing RUN apt-get
lines but run them against the new minimal base and remove any leftover
build-tool assumptions from the runtime stage.

ops/docker-sequencer-test/Dockerfile.l2-node-test (1)

37-47: Consider using a minimal runtime image (consistent with geth Dockerfile).

Same as the geth test Dockerfile, the runtime stage uses the full builder image. For consistency and smaller images, consider using a minimal base since only the compiled binaries are needed at runtime.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/Dockerfile.l2-node-test` around lines 37 - 47, The
final stage currently uses the full builder image (FROM
ghcr.io/morph-l2/go-ubuntu-builder:go-1.24-ubuntu) even though only the compiled
binaries are needed; change the final stage to a minimal runtime base (e.g., a
distroless or slim image) and re-add any required runtime packages
(ca-certificates or libc) if needed, then keep the COPY lines (COPY
--from=builder /bitget/morph/node/build/bin/tendermint /usr/local/bin/ and COPY
--from=builder /bitget/morph/node/build/bin/morphnode /usr/local/bin/) and the
CMD (["morphnode","--home","/data"]) so only the two binaries are included in
the smaller image.

ops/docker-sequencer-test/scripts/tx-generator.sh (1)

31-36: Code duplication with run-test.sh.

This get_block_number function is nearly identical to the one in run-test.sh (lines 53-59). Consider extracting shared utilities into a common script that both can source to avoid maintenance drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ops/docker-sequencer-test/scripts/tx-generator.sh` around lines 31 - 36, The
get_block_number function is duplicated between tx-generator.sh and run-test.sh;
extract this function into a shared utility script (e.g., a sourced common util
script) and replace the duplicate definitions by sourcing that shared script
from both tx-generator.sh and run-test.sh, ensuring the function name
get_block_number remains unchanged so callers keep working and update any
shebangs/execute permissions as needed.

node/l1sequencer/tracker.go (1)

54-68: Consider an immediate check on startup.

The loop only triggers on ticker ticks, so the first sync lag check happens 1 minute after start. Consider adding an immediate check before entering the loop to detect issues earlier.

🔧 Suggested improvement

 func (t *L1Tracker) loop() {
 	defer close(t.stop)
 
+	// Immediate check on startup
+	t.checkL1SyncLag()
+
 	ticker := time.NewTicker(1 * time.Minute)
 	defer ticker.Stop()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@node/l1sequencer/tracker.go` around lines 54 - 68, The loop in L1Tracker.loop
delays the first sync check by up to one minute; call t.checkL1SyncLag() once
immediately before entering the for/select loop so the first check runs on
startup (ensure you still respect t.ctx.Done() by returning if context is
canceled), then keep the existing ticker-driven checks; reference the
L1Tracker.loop function and the checkL1SyncLag method to locate where to add the
immediate invocation.

node/flags/flags.go (1)

3-6: Import order should follow Go conventions.

Standard library packages should be grouped before third-party packages. Consider reordering:

 import (
-	"github.com/urfave/cli"
 	"time"
+
+	"github.com/urfave/cli"
 )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@node/flags/flags.go` around lines 3 - 6, The import block in flags.go
violates Go convention by placing the third-party package github.com/urfave/cli
before the standard library package time; reorder the imports so standard
library packages (time) appear first, then a blank line, then third-party
packages (github.com/urfave/cli) to follow Go import grouping conventions.

contracts/contracts/l1/L1Sequencer.sol (1)

9-13: Consider adding a storage gap for future upgrades.

For upgradeable contracts, it's a best practice to reserve storage slots for future state variables. Without a gap, adding new variables in future upgrades could corrupt existing storage layout.

🛡️ Suggested addition

 contract L1Sequencer is OwnableUpgradeable {
     // ============ Storage ============
 
     /// `@notice` Current sequencer address
     address public sequencer;
+
+    /// `@dev` Reserved storage gap for future upgrades
+    uint256[49] private __gap;
 
     // ============ Events ============

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/contracts/l1/L1Sequencer.sol` around lines 9 - 13, The contract
L1Sequencer (which inherits OwnableUpgradeable) lacks a reserved storage gap and
so adding new state vars in upgrades could corrupt the storage layout; add a
private reserved storage array (e.g., uint256[n] __gap) after the declared state
variable `address public sequencer;` (inside the storage section of L1Sequencer)
to reserve future slots and preserve compatibility with OwnableUpgradeable and
future upgrades.

node/core/executor.go (1)

662-683: Consider consistent nil Transactions handling in executableL2DataToBlockV2.

blockV2ToExecutableL2Data (lines 639-643) ensures Transactions is never nil, but executableL2DataToBlockV2 doesn't apply the same treatment. While this may be fine if the caller always provides non-nil data, it's inconsistent.

♻️ Suggested consistency improvement

 func executableL2DataToBlockV2(data *catalyst.ExecutableL2Data) *l2node.BlockV2 {
 	if data == nil {
 		return nil
 	}
+	txs := data.Transactions
+	if txs == nil {
+		txs = make([][]byte, 0)
+	}
 	return &l2node.BlockV2{
 		ParentHash:         data.ParentHash,
 		Miner:              data.Miner,
 		Number:             data.Number,
 		GasLimit:           data.GasLimit,
 		BaseFee:            data.BaseFee,
 		Timestamp:          data.Timestamp,
-		Transactions:       data.Transactions,
+		Transactions:       txs,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@node/core/executor.go` around lines 662 - 683, executableL2DataToBlockV2
currently copies Transactions directly and can leave BlockV2.Transactions nil,
making it inconsistent with blockV2ToExecutableL2Data which ensures Transactions
is never nil; update executableL2DataToBlockV2 to defensive-copy or set
Transactions to an empty slice when data.Transactions is nil (i.e., ensure
BlockV2.Transactions = data.Transactions or make([]<txnType>, 0) if nil) so both
conversion functions guarantee a non-nil Transactions slice.

node/l1sequencer/verifier.go (1)

59-83: IsSequencer logic has potential for double RPC call.

The current flow can make two L1 RPC calls in a single IsSequencer invocation:

1. Line 67: Refresh if cache expired
2. Line 78: Force refresh on cache miss

While this ensures correctness (catching recent sequencer changes), be aware of the RPC overhead. With a 10-second TTL, this is manageable, but with a longer TTL the double-call on miss becomes more relevant.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@node/l1sequencer/verifier.go` around lines 59 - 83, IsSequencer can perform
two RPC calls by calling flushCache twice (once for expired cache and again on
cache miss); modify IsSequencer to avoid a redundant second refresh by tracking
whether a refresh already occurred in this invocation—call flushCache once if
cache is expired, set a local flag (e.g., refreshed), check the cached sequencer
(c.sequencer) and only call flushCache again on miss if refreshed is false; keep
the existing mutex locking and return semantics and reference functions/fields
IsSequencer, flushCache, c.cacheExpiry, c.sequencer, and c.mutex.

node/l1sequencer/signer.go (1)

65-71: Consider whether nil verifier should be an error or expected state.

The IsActiveSequencer method returns an error when verifier is nil. However, NewLocalSigner accepts a nil verifier without error. This creates an asymmetry where construction succeeds but a key method fails.

If a nil verifier is a valid configuration (e.g., verifier disabled), consider returning (false, nil) instead. If a verifier is always required for proper operation, validate this in the constructor.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@node/l1sequencer/signer.go` around lines 65 - 71, The constructor/usage
mismatch: NewLocalSigner currently accepts a nil verifier but IsActiveSequencer
treats nil verifier as an error; pick one of two fixes and implement it
consistently: (A) If verifier is optional, change LocalSigner.IsActiveSequencer
to return (false, nil) when s.verifier == nil (remove the fmt.Errorf path) so
caller sees "not active" without error; or (B) If verifier is required, validate
in NewLocalSigner (e.g., return an error) and ensure NewLocalSigner does not
construct a LocalSigner with a nil verifier. Refer to the LocalSigner type, the
IsActiveSequencer method, and NewLocalSigner to locate where to apply the
change.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Configuration used: defaults

Review profile: CHILL

Plan: Pro